Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kimiko: add encoders #521

Merged
merged 6 commits into from
Aug 18, 2023
Merged

kimiko: add encoders #521

merged 6 commits into from
Aug 18, 2023

Conversation

cstrahan
Copy link
Contributor

This enables rotary encoder support for the keycapsss/kimiko keyboard.

kimiko

In keyboards/keycapsss/kimiko/keymaps/vial/rules.mk I set ENCODER_MAP_ENABLE = yes and built with:

CONVERT_TO=kb2040 make keycapsss/kimiko:vial

The buttons were laid out so that it reads (effective) counter-clockwise followed by clockwise (left to right).

@cstrahan
Copy link
Contributor Author

If the firmware is too big with encoders enabled, I can revert the second of the two commits.

@cstrahan
Copy link
Contributor Author

Last commit (7ec67dd) properly fixes the orientation of the rotary encoder:

kimiko

Copy link
Contributor

@lesshonor lesshonor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the firmware is too big with encoders enabled, I can revert the second of the two commits.

You should already have an idea of whether this is the case by building it. (Without the CONVERT_TO.)

If it is too big, selectively disabling large or less-common features for AVR is a better idea than reverting encoder mapping, ex:

ifeq ($(strip $(MCU)), atmega32u4)
QMK_SETTINGS = no
TAP_DANCE_ENABLE = no
KEY_OVERRIDE_ENABLE = no
GRAVE_ESC_ENABLE = no
SPACE_CADET_ENABLE = no
MAGIC_ENABLE = no
endif

#if defined(__AVR_ATmega32U4__)
#undef LOCKING_SUPPORT_ENABLE
#undef LOCKING_RESYNC_ENABLE
#undef RGBLIGHT_EFFECT_KNIGHT
#undef RGBLIGHT_EFFECT_CHRISTMAS
#undef RGBLIGHT_EFFECT_RGB_TEST
#undef RGBLIGHT_EFFECT_ALTERNATING
#undef RGBLIGHT_EFFECT_TWINKLE
#endif

Copy link
Contributor

@lesshonor lesshonor Aug 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert changes to this file. The Vial keymap submission is not an appropriate place to modify pin definitions for boards that were submitted to and still exist in upstream QMK.

To summarize comments I left on a previous PR:

  • if the encoders are wrong for everyone, you should fix them for everyone (i.e.: in QMK)
  • if your particular encoder is backwards because you sourced it yourself vs using the included/intended hardware, add #define ENCODER_DIRECTION_FLIP to your keymap's config.h
  • if the manufacturer couldn't be bothered to source consistent hardware so different runs of the keyboard use incompatible encoders...I'd probably just shrug my shoulders and let people fend for themselves. But you could consider adding additional layouts that allow end-users to flip the encoder directions in the GUI.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the encoders are wrong for everyone, you should fix them for everyone (i.e.: in QMK)

I agree -- that was a late modification made to this PR once I discovered that you could tweak the pin definitions between left/right hand halves. But this really should go upstream.

I can revert that part for now. Is it alright if I leave the vial.json as I have it currently (wrt encoders)? Or how would you like that handled? Once (/if) the info.json fix goes upstream, the CCW/CC orientation in the UI will be as one would expect.

if the manufacturer couldn't be bothered to source consistent hardware so different runs of the keyboard use incompatible encoders...I'd probably just shrug my shoulders and let people fend for themselves. But you could consider adding additional layouts that allow end-users to flip the encoder directions in the GUI

I think this is more a matter of the keyboard reusing the same PCB for left- and right-hand halves. The encoder is seated into the same through-holes, but on opposite sides. With the way the rest of the PCB laid out, this results in the encoder pins being mirrored between the two halves.

At any rate, I'll see if I can get the pin definitions merged upstream, and also ping the designer of the keyboard.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not particularly fussed about the precise accuracy of the vial.json, as someone who does not own this board and is unlikely to investigate it thoroughly. Reverse the directions now and make a follow-up PR to Vial later, or don't. In either case the keymap is more functional than before, so it gets my nod.

Funnily enough, BenRoe is actually submitting Kimiko rev2 to QMK right now and that submission requires changes to rev1—so your timing really couldn't be better.

@cstrahan
Copy link
Contributor Author

@lesshonor How does this look now?

When I run make keycapsss/kimiko:vial, I see:

Checking file size of keycapsss_kimiko_rev1_vial.hex                                                [OK]
 * The firmware size is fine - 27392/28672 (95%, 1280 bytes free)

so I think firmware size should be fine.

@cstrahan
Copy link
Contributor Author

It looks like the upstream info.json is going to be fixed soon: qmk/qmk_firmware#21728

@cstrahan
Copy link
Contributor Author

Is there any preference/precedent for the encoder "switches" layout, horizontal vs vertical? They are currently horizontal/side-by-side, but I could tweak it they are stacked vertically, with CW on top and CCW below, both positioned above the physical encoder push button switch.

Also, do we need a maintainer to approve? I see

1 workflow awaiting approval
This workflow requires approval from a maintainer.

@xyzz xyzz merged commit a4bba72 into vial-kb:vial Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants